-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bundler plugins #12
Bundler plugins #12
Conversation
Needs a little bit of cleanup and documentation, but I think I now have really strong 2nd MVP. |
const headersMiddleware: Get<[], UniversalMiddleware> = | ||
() => (_request, ctx) => { | ||
return (response) => { | ||
response.headers.set("X-Custom-Header", ctx.something ?? "NONE"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't review everything, but I'll be honest, stuff like this rubs me all the wrong ways - requiring optional fields in the context type, and then using the ??
operator, it just sounds too brittle for my taste. Users will put those middlewares in the wrong order, things will quietly malfunction - because the types aren't really telling the truth, it will be difficult for them to understand why.
I really want to see type safety for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type safety can be achieved with a proper webroute-adapter
for instance.
Proper type safety cannot be achieved currently with the current state of javascript servers (without something like webroute
), which is out of scope here.
The scope of this PR is to ease the work of tool authors, and the whole middleware composition stack should not be a part of this solution, as it adds a lot of overhead, maintainability cost, and complexity when explaining what this project purpose is.
It's still a huge step forward in the right direction. And again, complete type safety will be possible with webroute
and the servers it supports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could perhaps add the webroute
adapter in this PR, so that we can review it as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still a huge step forward in the right direction.
Yeah, no doubt. 🙂
And again, complete type safety will be possible with
webroute
and the servers it supports.
How?
If this universal middleware has inaccurate context types, how would webroute
solve that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm currently working on some improvements and the webroute
adapter to make it clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example using webroute adapter
The middleware signature has been adapted to correspond to this comment.
@sinclairnick as you can see, declaring middlewares outside @webroute/router
flow makes it a little bit verbose to actually extract the correct types.
I wonder if there would be a solution where webroute
could automatically extract required TState
("input" state) and generated DataResult
("output" state) for middlewares declared outside of webroute flow.
For instance, I could declare a middleware with the following signature WebrouteMiddleware<InState, OutState = OutState>
and have webroute
extract InState
and OutState
if the signature matches WebrouteMiddleware
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies if I've misunderstood here, as there's obviously a lot of context/info in this repo I may have missed.
But if I understand correctly, there is indeed a type that's allows declaring the inputs and outputs, without having to rely on inference.
The MiddlewareFn type enables setting:
- The "data result"
- The middleware's extra input parameters
- The "response handler"'s extra parameters
export type MiddlewareFn<
T extends DataResult | void = void,
TRest extends any[] = any[],
TRestRes extends any[] = any[]
> = (
request: Request,
...rest: TRest
) => Awaitable<MiddlewareResult<T, TRestRes>>;
So to achieve what you're describing this would look something like:
type MyMiddleware = MiddlewareFn<OutputCtx, [InputCtx], []>
// OutputCtx is the return type, InputCtx is a single extra param, no response handler extra params
Obviously a small wrapper type could bend this into the correct order of params etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sinclairnick thanks, that's a good start, but I realize that I should make a reproduction repo. My issue seems to be more related to the interaction between MiddlewareFn
from @webroute/middleware
then using it inside @webroute/route
route().use(...)
. But I think that we'll hit a Typescript limitation for what I have in mind...
End goal
For tool authors
Declare middlewares and handlers once, target all supported servers:
For users
Easy usage for supported servers:
TODO